-
Notifications
You must be signed in to change notification settings - Fork 33
fix: isRemoteControlEnabled sync
#236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds a mutex-protected Changes
Sequence DiagramsequenceDiagram
participant RS as RoomService
participant RH as RoomEventHandler
participant P as Participant (late-joiner)
Note over RS: PublishTrack / PublishControllerCursorEnabled updates rc state
RS->>RS: Set remote_control_enabled (Mutex)
RS->>RH: (events stream) room events task has access to inner
P->>RH: ParticipantConnected
activate RH
RH->>RS: Read remote_control_enabled (lock)
RS-->>RH: remote_control_enabled value
RH->>P: Publish RemoteControlEnabled event
deactivate RH
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I was cleaning up this and while testing it hit a limitation of this approach. There is a race on processing the event in the controller. I had disabled remote control but when the controller joined the room no toaster or indication was shown. Reloading made the icon on the top right to appear. Maybe we should store this in the livekit room metadata instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tauri/src/components/SharingScreen/SharingScreen.tsx (1)
130-156:⚠️ Potential issue | 🟡 MinorGuard reads stale closure state and can skip legitimate transitions.
If two messages arrive before a re-render completes, the second state transition may be dropped. The callback closes over the render-time
isRemoteControlEnabledvalue. WhenupdateCallTokenstriggers a store update, there's a window where rapid follow-up messages still use the old captured value in the guard check.Compare against a ref that tracks the latest value instead:
🔧 Suggested fix using a ref for the latest value
const { updateCallTokens, callTokens } = useStore(); const isRemoteControlEnabled = callTokens?.isRemoteControlEnabled ?? false; + const isRemoteControlEnabledRef = useRef(isRemoteControlEnabled); + useEffect(() => { + isRemoteControlEnabledRef.current = isRemoteControlEnabled; + }, [isRemoteControlEnabled]); useDataChannel("remote_control_enabled", (msg) => { const decoder = new TextDecoder(); const payload: TPRemoteControlEnabled = JSON.parse(decoder.decode(msg.payload)); const newValue = payload.payload.enabled; // Skip if value matches current state - if (newValue === isRemoteControlEnabled) { + if (newValue === isRemoteControlEnabledRef.current) { return; }
|
TODO: Refactor this to use Livekit's Room metadata. Check if metadata are persistent or not (case for our Rooms implementation because ID is the same) |
AI generated with fixes from me 👇
Summary
Fixes issue #233 (
isRemoteControlEnabledis not correct with new participants).This PR ensures that late-joining participants receive the correct remote-control state from the sharer, rather than incorrectly assuming remote control is enabled.
Two-pronged fix:
Core (Rust): The sharer now tracks remote-control state internally and rebroadcasts it when new participants join, ensuring late joiners receive the current state immediately
Client (TypeScript): Default
isRemoteControlEnabledistrueacross all join flows. Ideally it would befalse, but it creates a small "glitch" with a label of having remote control disabled in the session which I didn't like.Changes
Core (room_service.rs)
is_sharerflag to track if this instance is the screen-sharing authorityremote_control_enabledstate to track current remote-control settingPublishTrack(share start): mark as sharer and publish initial remote-control stateRoomEvent::ParticipantConnected: if sharer, rebroadcast current state to ensure late joiner syncPublishControllerCursorEnabled: update internal state for future rebroadcastsConcerns
Currently we have this code which resides inside
SharingScreen.tsx, which I think has a probability of missing/dropping a message from sharer, if it will get created a bit slower.This part, as it updates the call state, it should live inside the main app's window, so we avoid any race conditions.
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.